Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config cleanup #1903

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Config cleanup #1903

merged 1 commit into from
Jun 28, 2016

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jun 24, 2016

  • Code cleanup
  • Add humanize for buffersize

@ruflin ruflin added in progress Pull request is currently in progress. Filebeat Filebeat labels Jun 24, 2016
@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Jun 24, 2016
@ruflin ruflin force-pushed the config-cleanup branch 2 times, most recently from 112383d to c07e922 Compare June 27, 2016 07:18
@@ -22,7 +22,9 @@ type Filebeat struct {

// New creates a new Filebeat pointer instance.
func New() *Filebeat {
return &Filebeat{}
return &Filebeat{
config: &cfg.DefaultConfig,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't take the default config as pointer. DefaultConfig normally serves as template to be copied and must not contain any pointers!

Rather allocate new config initialized with DefaultConfig like:

config := cfg.DefaultConfig
return &Filebeat{ config: &config }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ruflin
Copy link
Contributor Author

ruflin commented Jun 27, 2016

@urso New version pushed.

PublishAsync bool `config:"publish_async"`
IdleTimeout time.Duration `config:"idle_timeout"`
IdleTimeout time.Duration `config:"idle_timeout" validate:"nonzero,min=0"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min=1 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if I want to disable idle_time, e.g. via -1 or 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that is something that we should encourage. That above allows to set it for example to 0.001s which is almost like disabling it.

One thing I just realised related to duration and ucfg: it should be min=1s and not min=1 as if the unit is not know, it could be "anything".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right. if not setting any unit, the default is 's'. But normally I set units.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in past `idle_timeout' was disablable, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@urso didn't know it allows to set units there. Nice :-) Will add units.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default before (and now) is 5s

* Code cleanup
* Add humanize for buffersize
@urso urso merged commit f9d028e into elastic:master Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants